Add custom error types in uxarray - #1621
Conversation
Adds DataCenteringError, DimensionError, GridInvalidError, and GridsMismatchError. Replaces existing errors with one of these where appropriate AND where it will not break backwards compatibility. Kept notes (will include in PR writeup) about places where it would break compatibility, and did not change those places yet.
spot check that custom error types are working: changes two existing tests to catch custom error types. Does not change many other tests, because it is also desirable to confirm that these changes are actually backwards-compatible, by not messing with the test suite too much.
Can we add them as warnings instead? It should be easy to promote them to errors later. I just lean towards having the messages present in some form. |
Thank you for taking a look! To clarify, the locations specified here are already errors on the
what I mean is: the code currently raises But, I'm not sure if those replacements should be added in this PR, since they break backwards compatibility in a larger way than the other changes (e.g., GridInvalidError is not a subclass of RuntimeError, so changing the error type in this example would prevent it from being caught by existing |
|
Ah, yeah, that makes sense. |
|
Alright, I tried out raising error messages with each of them, and they all raise with the correct error type. I'm going to assume that exhaustively triggering all the new callsites is overkill, so I'm approving this. |
Closes #1556
Overview
Adds custom error types in uxarray. Defining custom error types can help with:
In particular, this PR adds the following error types:
Small expansion of scope
This PR expands scope slightly beyond that of the original issue as follows:
YacNotAvailableErrorfrom uxarray/remap/yac.py to uxarray/errors.py.The following justification for this move is provided in the docstring at the top of errors.py:
Though, this could be reverted if PR reviewers disagree or feel that this should be scoped as a separate issue instead.
Compatibility notes:
This PR should be fully backwards compatible, or nearly fully backwards compatible. Compatibility is maintained via subclassing. For example, any code catching ValueError will still catch any of the newly-defined custom error types, because they inherit from ValueError.
This PR adjusts two tests to check that custom error types are indeed being raised (e.g.
with pytest.raises(DataCenteringError)instead ofwith pytest.raises(ValueError)). It does not adjust all tests to the new error types because it is desirable to see that the existing tests still pass, to confirm there are no breaking changes here.Thinking about it a bit further, there are three technically-possible breakage points for existing code, but only for hopefully-very-unlikely code design patterns:
repr(err).startswith("ValueError")type(err) == ValueErrorisinstance(err, Exception) and not isinstance(err, ValueError), could fail in some cases. This PR replaces Exception with a custom error type in some cases, such as using DataCenteringError instead of Exception in a few places in grid.py.Other Recommended Changes (Not Implemented Here)
Should custom errors become part of the public API? I would be interested in at least adding
import uxarray.errorsinuxarray.__init__.py, so that users who have imported uxarray already can do things like:ux.errors.DimensionErrorwithout needing to remember to add another import statement likefrom uxarray.errors import DimensionError.There are a few locations where the new error types would better describe the actual error, but which are not included in this PR because they could break backwards compatibility. These could be added here if PR reviewers want, or split into a separate issue and PR afterwards. Including these here is not necessary to close issue #1556.
I also put together a variety of error-type changes I would recommend but which are unrelated to this PR, because they do not use the new error types. (Example: ValueError → TypeError in grid.utils.make_setter: f"{key} must be an xr.DataArray", which gets raised if not isinstance(value, xr.DataArray).) I have opened a separate issue to track those, see #1622.
PR Checklist
General
Testing
Documentation
_) and have been added todocs/internal_api/index.rstdocs/user_api/index.rst